Codecov Report

Attention: Patch coverage is 0% with 10 lines in your changes missing coverage. Please review.

Project coverage is 37.59%. Comparing base (c00e94f) to head (8538dd1).
Report is 1 commits behind head on main.

@@            Coverage Diff             @@
##             main     #277      +/-   ##
==========================================
- Coverage   37.80%   37.59%   -0.21%     
==========================================
  Files          19       19              
  Lines        3214     3221       +7     
==========================================
- Hits         1215     1211       -4     
- Misses       1999     2010      +11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Thanks for your contribution. I don't understand the security aspect that is claimed to exist here. Is this just about the alleged time.it takes to scan a code vs the time it takes to copy a standard 2 word code by hand?

First, thank you for taking the time to look over my pr.

Thanks for your contribution. I don't understand the security aspect that is claimed to exist here. Is this just about the alleged time.it takes to scan a code vs the time it takes to copy a standard 2 word code by hand?

Yeah more or less it's just easier to scan a qr code, which is done automatically, than taking the time to copy a code. I don't think it's that big of a deal at the end of the day, I just also like having the option of changing the default.

It seems as though the maintainers are leaning towards qr code by default, which has the benefit of being better for those who do not mind this default and usage being consistent across multiple versions of magic-wormhole-cli. I made another branch that keeps the same default as before unless you use the --no-qr flag, and will leave which you would like to implement up to your discretion. If you do prefer qr code by default, I can make a new PR, merge/rebase the other branch onto this one, or however you'd want me to do it.

Alright, as the python client went with QR code as default I think this is what we should do too :)

If anything else because we are already doing this, and I like to adhere to semver backwards compatibility for the CLI as well, as the version numbers are linked.

I can make a new PR, merge/rebase the other branch onto this one, or however you'd want me to do it.

It doesn't matter much to me. Force push this one, create a new PR and close this one, whatever is best for you.

While we are touching this part, I was wondering if the PrintCodeFn arguments could be factored into a struct that's shared with parse_and_connect (they share almost the same arguments, but not quite). It's getting a bit messy with all these arguments. But that's optional.

Ok, I forced pushed my changes from the other branch so it's now qr by default. I fully agree with your reasoning here. Made an empty commit just to re-trigger the failed coverage test and then it work. Weird.

While we are touching this part, I was wondering if the PrintCodeFn arguments could be factored into a struct that's shared with parse_and_connect (they share almost the same arguments, but not quite). It's getting a bit messy with all these arguments. But that's optional.

I agree it was a bit weird the way things work with PrintCodeFn and can probably be done in a cleaner way. I was initially trying to pass the full CommonLeaderArgs struct into parse_and_connect and then quickly realized that there were all these other arguments in there and that sender_print_code relied on PrintCodeFn and decided that I wanted to make as few changes as possible to make this work. Accept these changes for now. I will create a new issue to reference this so at least it's out there, and then it can be addressed in a future PR.